-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests to tips example #833
Conversation
ℹ️ It tests if the tips is loaded and can be opened. Then it submits three tips and sees if it gets the correct response.
⏰ Added time to toxicity detection timeout as the test is not passing reliably in the github action
Codecov Report
@@ Coverage Diff @@
## main #833 +/- ##
==========================================
- Coverage 64.61% 64.48% -0.13%
==========================================
Files 107 107
Lines 9281 9281
==========================================
- Hits 5997 5985 -12
- Misses 3284 3296 +12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the test here seems great, thanks for adding it! One note on the added script
@@ -0,0 +1,24 @@ | |||
from mephisto.abstractions.databases.local_database import LocalMephistoDB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should go in a gh_actions
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments, but overall LGTM
pip install -e . | ||
pip uninstall detoxify <<EOF | ||
y | ||
EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protip: there's a command called yes
that you can use here instead:
yes | pip uninstall detoxify
NAME
yes – be repetitively affirmative
SYNOPSIS
yes [expletive]
DESCRIPTION
The yes utility outputs expletive, or, by default, “y”, forever.
cy.get('[data-cy="directions-container"]'); | ||
cy.get('[data-cy="task-data-text"]'); | ||
cy.get('[data-cy="good-button"]'); | ||
cy.get('[data-cy="bad-button"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these could be suffixed with .should('exist')
for readability and clearer assertions. this also bakes in some retry semantics...
another option is .should('be.visible')
if we want to also check for cases where the element is visible (and not being pushed off screen or hidden by CSS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that said, i realize that .get by itself does error out if the element isn't found, so perhaps it's not as necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think it is necessary. I mainly use cy.should('not.exist')
to check for content that is not on the page, but I don't really know the point of using cy.should('exist')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it seems it's redundant, it maybe just indicates intention that an assertion is taking place. i'm fine with keeping it as is
|
||
""" | ||
This script should not be used locally. | ||
It is only used in a GitHub action as it does not automatically expire units. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this script seems very "one-off"-y. is there no other case we may want to intentionally force expire units? I assume it could be useful? I would consider parametizing the task_name below and having this be a general script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any situation outside of the GitHub action where you would want to run a script to expire units. Usually you would expire units by just doing ctrl-c two or three times on your mephisto task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Force expiring" units is generally done with a cleanup
script, which is provider-associated. This only updates the local status, and is a particular anti-pattern outside of the context of eval scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it... if this script is really needed for a test, I guess I'm okay with keeping it then
Overview
Adds three cypress end to end tests for the
static-react-task-with-tips
task example.Pre-tip submission
This tests if the tips popup can be opened. It does error checking by typing messages that are too long. It also submits three tips and checks if the server response is correct.
Video:
pre-tip-submission.mp4
Feedback submission
Checks if the correct elements are present and submits answers to the two feedback questions in the example.
Video:
feedback-submission.mov
Post-tip submission
After the pre-tip submission test there are three tips that need to be reviewed. Prior to running the post-tip submission test make sure to run
python review_tips_for_task.py
and accept the first tip and reject the other two tips.The test checks to see if the first submitted tip from the pre-tip submission test is present.
Video:
post-tip-submission.mov
GitHub action file
Adds a new job to run the cypress test above.
mephisto-task
andmephisto-worker-addons
packages.expire_all_units.py
scriptreview_tips_for_task.py
script.remove_accepted_tip.py
.review_feedback_for_task.py